[SPARK-23549][SQL] Cast to timestamp when comparing timestamp with date #20774

kiszk · 2018-03-08T18:26:00Z

What changes were proposed in this pull request?

This PR fixes an incorrect comparison in SQL between timestamp and date. This is because both of them are casted to string and then are compared lexicographically. This implementation shows false regarding this query spark.sql("select cast('2017-03-01 00:00:00' as timestamp) between cast('2017-02-28' as date) and cast('2017-03-01' as date)").show.

This PR shows true for this query by casting date("2017-03-01") to timestamp("2017-03-01 00:00:00").

(Please fill in changes proposed in this fix)

How was this patch tested?

Added new UTs to TypeCoercionSuite.

kiszk · 2018-03-08T18:26:25Z

If this approach is fine, I will create tests for .sql.

SparkQA · 2018-03-08T22:04:16Z

Test build #88101 has finished for PR 20774 at commit 21368b4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-03-09T01:20:19Z

cc: @gatorsmile

gatorsmile · 2018-03-12T06:18:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

Although this PR makes sense, we still need a SQLConf. Also add this behavior change into the migration guide in the SQL doc

I see. I will add a SQLConf. Is there any suggestion for a name of this entry?

viirya · 2018-03-12T07:26:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

So it means TimeStamp('2017-03-01 00:00:01') eq Date('2017-03-01') = false? Looks a bit weird.

At least this comparison does not get true in Hive:

hive> select cast('2017-03-01 00:00:00' as timestamp) = cast('2017-03-01' as date); OK false

Thank you for your try.

The current implementation follows this answer.

Which version of hive did you use?

Thanks
@dongjoon-hyun are you pointing out this HIVE JIRA entry in this comment? Or other HIVE JIRA entries?

Seems it's fixed in Hive 2.2.

I think so, but @dongjoon-hyun 's comment said it is fixed at 2.0.

SparkQA · 2018-03-18T07:05:02Z

Test build #88346 has finished for PR 20774 at commit a16deaa.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

kiszk · 2018-03-18T10:39:10Z

retest this please

SparkQA · 2018-03-18T14:06:17Z

Test build #88348 has finished for PR 20774 at commit a16deaa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

SparkQA · 2018-03-18T20:14:00Z

Test build #88357 has finished for PR 20774 at commit 5fbbc30.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-03-19T01:47:21Z

cc @gatorsmile

gatorsmile

I verified it in DB2. It works as what this PR proposes.

gatorsmile · 2018-03-19T05:01:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

private def findCommonTypeForBinaryComparison( dt1: DataType, dt2: DataType, conf: SQLConf): Option[DataType] = (dt1, dt2) match {

gatorsmile · 2018-03-19T05:02:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

two more spaces before if

gatorsmile · 2018-03-19T05:07:41Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala

test("binary comparison with string promotion") { val rule = TypeCoercion.PromoteStrings(conf) ruleTest(rule, GreaterThan(Literal("123"), Literal(1)), GreaterThan(Cast(Literal("123"), IntegerType), Literal(1))) ruleTest(rule, LessThan(Literal(true), Literal("123")), LessThan(Literal(true), Cast(Literal("123"), BooleanType))) ruleTest(rule, EqualTo(Literal(Array(1, 2)), Literal("123")), EqualTo(Literal(Array(1, 2)), Literal("123"))) ruleTest(rule, GreaterThan(Literal("1.5"), Literal(BigDecimal("0.5"))), GreaterThan(Cast(Literal("1.5"), DoubleType), Cast(Literal(BigDecimal("0.5")), DoubleType))) Seq(true, false).foreach { convertToTS => withSQLConf("spark.sql.hive.compareDateTimestampInTimestamp" -> convertToTS.toString) { val date0301 = Literal(java.sql.Date.valueOf("2017-03-01")) val timestamp0301000000 = Literal(Timestamp.valueOf("2017-03-01 00:00:00")) val timestamp0301000001 = Literal(Timestamp.valueOf("2017-03-01 00:00:01")) if (convertToTS) { // `Date` should be treated as timestamp at 00:00:00 See SPARK-23549 ruleTest(rule, EqualTo(date0301, timestamp0301000000), EqualTo(Cast(date0301, TimestampType), timestamp0301000000)) ruleTest(rule, LessThan(date0301, timestamp0301000001), LessThan(Cast(date0301, TimestampType), timestamp0301000001)) } else { ruleTest(rule, LessThan(date0301, timestamp0301000000), LessThan(Cast(date0301, StringType), Cast(timestamp0301000000, StringType))) ruleTest(rule, LessThan(date0301, timestamp0301000001), LessThan(Cast(date0301, StringType), Cast(timestamp0301000001, StringType))) } } } }

gatorsmile · 2018-03-19T05:08:57Z

sql/core/src/test/resources/sql-tests/inputs/predicate-functions.sql

A little hard to track the exact changes. Let us move all the new tests to the end.

+1 it is really confusing to look at the diff

gatorsmile · 2018-03-19T05:09:21Z

docs/sql-programming-guide.md

We also need to add it to migration guide.

gatorsmile · 2018-03-19T05:21:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

It should not be under hive, because it is not only for hive

spark.sql.typeCoercion.compareDateTimestampInTimestamp

gatorsmile · 2018-03-19T05:22:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

internal()?

perhaps mention this config will be removed in spark 3.0.

(on a related note we should look at those configs for backward compatibility and consider removing them in 3.0)

SparkQA · 2018-03-20T07:05:02Z

Test build #88403 has finished for PR 20774 at commit a3dc357.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-03-20T10:37:31Z

Test build #88412 has finished for PR 20774 at commit 4df6513.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

kiszk · 2018-03-20T16:20:30Z

Retest this please

SparkQA · 2018-03-20T17:55:03Z

Test build #88425 has finished for PR 20774 at commit 4df6513.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

kiszk · 2018-03-20T18:43:28Z

Retest this please

kiszk · 2018-03-20T19:32:09Z

Retest this please

SparkQA · 2018-03-20T22:11:03Z

Test build #88433 has finished for PR 20774 at commit 4df6513.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

SparkQA · 2018-03-20T23:07:54Z

Test build #88434 has finished for PR 20774 at commit 4df6513.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

SparkQA · 2018-03-25T21:57:42Z

Test build #88574 has finished for PR 20774 at commit 5ca2341.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PromoteStrings(conf: SQLConf) extends TypeCoercionRule
case class InConversion(conf: SQLConf) extends TypeCoercionRule

gatorsmile · 2018-03-25T23:37:58Z

LGTM

Thanks! Merged to master

gatorsmile reviewed Mar 12, 2018

View reviewed changes

viirya reviewed Mar 12, 2018

View reviewed changes

gatorsmile reviewed Mar 19, 2018

View reviewed changes

kiszk force-pushed the SPARK-23549 branch from a3dc357 to 4df6513 Compare March 20, 2018 08:05

address review comments

5ca2341

kiszk force-pushed the SPARK-23549 branch from 4df6513 to 5ca2341 Compare March 25, 2018 18:31

asfgit closed this in e4bec7c Mar 25, 2018

[SPARK-23549][SQL] Cast to timestamp when comparing timestamp with date #20774

[SPARK-23549][SQL] Cast to timestamp when comparing timestamp with date #20774

Uh oh!

Conversation

kiszk commented Mar 8, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

kiszk commented Mar 8, 2018

Uh oh!

SparkQA commented Mar 8, 2018

Uh oh!

kiszk commented Mar 9, 2018

Uh oh!

gatorsmile Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 18, 2018

Uh oh!

kiszk commented Mar 18, 2018

Uh oh!

SparkQA commented Mar 18, 2018

Uh oh!

SparkQA commented Mar 18, 2018

Uh oh!

kiszk commented Mar 19, 2018

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 20, 2018

Uh oh!

SparkQA commented Mar 20, 2018

Uh oh!

kiszk commented Mar 20, 2018

Uh oh!

SparkQA commented Mar 20, 2018

Uh oh!

kiszk commented Mar 20, 2018

Uh oh!

kiszk commented Mar 20, 2018

gatorsmile Mar 12, 2018 •

edited

Loading

kiszk Mar 12, 2018 •

edited

Loading

kiszk Mar 12, 2018 •

edited

Loading